Conversation
d-perl
added
the
needed_for_release
DiamondJoseph
left a comment
DiamondJoseph
left a comment
There was a problem hiding this comment.
Leaving as comment to not block anything, but I have some reservations.
We're not making use of configuration files (at least, currently) and the testing rightly makes that optional- but it still runs the tests twice for each non-mx beamline when we have no conditional devices, but if we begin adding optional beamlines which rely on things other than "am I the live beamline or not", e.g. devices which are sometimes removed from the beamline, devices which might be used in one profile but not others, we'll be unable to test that without completely rewriting these tests again.
| @@ -0,0 +1,299 @@ | |||
| # | |||
| # | |||
| BeamLine BL03I | |||
There was a problem hiding this comment.
Is all of this required for the tests?
BeamLine BL03S?
There was a problem hiding this comment.
I could chop some of it out, good point
| with patch.dict(os.environ, {"BEAMLINE": bl}, clear=True): | ||
| bl_mod = mock_bl(beamline) | ||
| devices = make_all_devices(bl_mod, fake_with_ophyd_sim=True) | ||
| for device_name, device in devices.items(): | ||
| assert device_name in beamline_utils.ACTIVE_DEVICES | ||
| assert follows_bluesky_protocols(device) | ||
| assert len(beamline_utils.ACTIVE_DEVICES) == len(devices) | ||
| beamline_utils.clear_devices() | ||
| del bl_mod |
There was a problem hiding this comment.
Not really extensible for @skip_device using other conditions that the value of beamline.BL.
Maybe testing the configurations when @skip_device is !x when it is default x should be in test_beamline, and this only test only the default case?
There was a problem hiding this comment.
I'm not sure I totally understood the second part of this comment but I agree with the first - instead I included skipped devices in these tests
|
Thanks @DiamondJoseph, I basically agree with your concerns. I updated the tests to only run once for each beamline, but create skipped devices instead. Unfortunately we do use data from these config files in instantiating several devices, so it was only because they were being skipped that tests could run at all. Even more annoyingly, because the default files are on the DLS filesystem they might only break in CI. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 89.97% 92.89% +2.92%
==========================================
Files 83 83
Lines 3171 3182 +11
==========================================
+ Hits 2853 2956 +103
+ Misses 318 226 -92 ☔ View full report in Codecov by Sentry. |
|
@dperl-dls Still getting failing tests #332 rebased on top of main I've seen it with p45 and i04, so there's still some state not being cleaned up properly. |
3 participants
Fixes #331
Improves the test infrastructure for dodal:
Instructions to reviewer on how to test:
Checks for reviewer